refactor: move tests to separate files #52
Conversation
tests became a bit hard to manage, so moving them into separate files. The integration tests were easy to move into tests/ dir, but the unit tests should be in a separate file because we don't want to grow public API surface
There was a problem hiding this comment.
Pull Request Overview
This PR reorganizes the test suite by moving integration tests from the main library module to separate test files. Unit tests that exercise private parser functions are moved to a new src/parser_tests.rs module file, while integration tests that use the public API are relocated to tests/parsing.rs. This refactoring improves code maintainability without expanding the public API surface.
Key changes:
- Created
tests/parsing.rsfor integration tests using public API - Created
src/parser_tests.rsfor unit tests of internal parser functions - Made several parser functions
pub(crate)to enable internal testing - Made
ValDescriptionstruct fields public to support integration test assertions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/parsing.rs | New integration test file containing tests that use the public Dbc API |
| src/parser_tests.rs | New unit test module containing parser function tests |
| src/lib.rs | Removed test module, added parser_tests module import, made ValDescription fields public |
| src/parser.rs | Removed inline test module, changed several parser functions from private to pub(crate) visibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let message_id = MessageId::Extended(2 ^ 29); | ||
| assert_eq!(message_id.raw(), 2 ^ 29 | 1 << 31); |
There was a problem hiding this comment.
The XOR operator ^ is being used where bitwise OR | is likely intended. 2 ^ 29 evaluates to 31, not a 29-bit shifted value. This should be 2 | (1 << 29) or 2 + (1 << 29) if setting bit 29.
| let message_id = MessageId::Extended(2 ^ 29); | |
| assert_eq!(message_id.raw(), 2 ^ 29 | 1 << 31); | |
| let message_id = MessageId::Extended(2 | 1 << 29); | |
| assert_eq!(message_id.raw(), (2 | 1 << 29) | 1 << 31); |
There was a problem hiding this comment.
I think this is a valid critique. There isn't a valid reason to xor 2 and 29.
There was a problem hiding this comment.
I guess we should just hardcode some value (in a separate PR) using hex notation?
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| #[test] | ||
| fn signal_comment_test() { | ||
| let def = r#" | ||
| CM_ SG_ 193 KLU_R_X "This is a signal comment test"; |
There was a problem hiding this comment.
Should we think about using an indo macro here to clean up formatting? https://docs.rs/indoc/latest/indoc/#using-indoc
There was a problem hiding this comment.
Funny - I was actually thinking initially of using it myself - it is great for multiline strings. That said, in this case I think it would be less than ideal because indoc introduces some string handling magic like removing prefix spaces on each line - nothing too big, but that makes the test string content less certain - which is not that great for a test case. In my examples, I only remove initial spaces of the first line, and the rest is exactly as written.
2 participants
tests became a bit hard to manage, so moving them into separate files. The integration tests were easy to move into tests/ dir, but the unit tests should be in a separate file because we don't want to grow public API surface